Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PLAT 638 Adjust login wih session and cookies #1180

Merged
merged 47 commits into from
Mar 24, 2023
Merged

PLAT 638 Adjust login wih session and cookies #1180

merged 47 commits into from
Mar 24, 2023

Conversation

nour-borgi
Copy link
Contributor

In this PR:

  • Added koa-passport
  • Added koa-session
  • Added koa-compose to handle multiple middlewares

Now authentication can be done by either "basic" or "local" types.
"local" means through the UI with username and password, this will create a session and set cookies in the browser.
"basic" means with basic auth either through browser or postman by giving also username and password.

In Mongo, added passport and session tables to store auth info.

@nour-borgi nour-borgi mentioned this pull request Jan 30, 2023
Merged
marrouchi
marrouchi requested changes Jan 31, 2023
View reviewed changes
Copy link
Collaborator

@marrouchi marrouchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @nour-borgi , left a couple of comments !

package.json Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/middleware/sessionStore.js Outdated Show resolved Hide resolved
src/model/passport.js Outdated Show resolved Hide resolved
src/passport.js Show resolved Hide resolved
src/protocols/local.js Outdated Show resolved Hide resolved
src/protocols/local.js Outdated Show resolved Hide resolved
marrouchi
marrouchi requested changes Feb 2, 2023
View reviewed changes
src/middleware/sessionStore.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
marrouchi
marrouchi previously approved these changes Feb 2, 2023
View reviewed changes
src/middleware/tlsAuthentication.js Outdated Show resolved Hide resolved
src/model/passport.js Outdated Show resolved Hide resolved
rcrichton
rcrichton requested changes Feb 7, 2023
View reviewed changes
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me. A few comments.

config/default.json Show resolved Hide resolved
src/api/authentication.js Show resolved Hide resolved
src/middleware/tlsAuthentication.js Outdated Show resolved Hide resolved
@nour-borgi nour-borgi mentioned this pull request Feb 8, 2023
Closed
rcrichton
rcrichton requested changes Mar 2, 2023
View reviewed changes
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on a quick scan, just one comment. I'l try go through and test it and give more feedback when I am able.

src/koaApi.js Outdated Show resolved Hide resolved
nour-borgi and others added 11 commits March 2, 2023 15:06
@nour-borgi
Remove key from koaApi.js
d19c4d6
@nour-borgi @bradsawadye
Update test/integration/auditAPITests.js
1372b80 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/passportTest.js
1364746 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/passportTest.js
ddb95ad 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/passportTest.js
8358ead 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/passportTest.js
825e799 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/usersTest.js
b497230 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/usersTest.js
d7f4abc 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/usersTest.js
ef2f2dd 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/unit/utilsTest.js
0100068 
Co-authored-by: bradsawadye <[email protected]>
@nour-borgi @bradsawadye
Update test/integration/generalAPITests.js
7f6d37e 
Co-authored-by: bradsawadye <[email protected]>
michaelloosen
michaelloosen reviewed Mar 17, 2023
View reviewed changes
src/api/authentication.js Outdated Show resolved Hide resolved
src/api/authentication.js Show resolved Hide resolved
rcrichton
rcrichton previously approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've tested this and all appears to work well. Just added a tiny comment.

src/api/authentication.js Outdated Show resolved Hide resolved
@nour-borgi
Merge pull request #1182 from jembi/PLAT-638-adjust-login-cookies-tes…
ab76407 
…t-fix

Plat 638 adjust login cookies test fix
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #1180 (927a69f) into master (5ece1a1) will increase coverage by 1.14%.
The diff coverage is 95.34%.

❗ Current head 927a69f differs from pull request most recent head ab76407. Consider uploading reports for the commit ab76407 to get more accurate results

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   85.08%   86.23%   +1.14%     
==========================================
  Files          77       84       +7     
  Lines        5310     5716     +406     
==========================================
+ Hits         4518     4929     +411     
+ Misses        792      787       -5
Impacted Files Coverage Δ
src/server.js 73.17% <69.23%> (+0.01%) ⬆️
src/upgradeDB.js 82.31% <82.75%> (+0.22%) ⬆️
src/api/users.js 92.54% <91.93%> (+9.02%) ⬆️
src/middleware/sessionStore.js 93.75% <93.75%> (ø)
src/passport.js 96.00% <96.00%> (ø)
src/api/authentication.js 95.60% <96.66%> (-0.13%) ⬇️
src/api/transactions.js 90.51% <100.00%> (+0.03%) ⬆️
src/koaApi.js 100.00% <100.00%> (ø)
src/model/index.js 100.00% <100.00%> (ø)
src/model/passport.js 100.00% <100.00%> (ø)
... and 7 more

... and 65 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nour-borgi nour-borgi merged commit 1a3e390 into master Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradsawadye bradsawadye bradsawadye left review comments

@michaelloosen michaelloosen michaelloosen approved these changes

@marrouchi marrouchi marrouchi left review comments

@rcrichton rcrichton rcrichton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nour-borgi @rcrichton @marrouchi @bradsawadye @michaelloosen